Dead Page Reference Count Bug Fix #181
Conversation
bhaskarbora2
changed the title
There was a problem hiding this comment.
General comment: on functions that return Status or StatusOr, usually it's best to declare as noexcept. My advice would be to do this on functions whose signature we modify, or newly added functions; as opposed to just going through and changing a bunch of existing signatures.
| @@ -55,13 +55,21 @@ class PageRecycler | |||
| CountMetric<u64> page_drop_error_count{0}; | |||
| }; | |||
|
|
|||
| struct GlobalMetrics { | |||
| CountMetric<u32> page_id_deletion_reissue_count{0}; | |||
There was a problem hiding this comment.
Why not u64 here? Not that we expect a lot of these, but why not?
There was a problem hiding this comment.
My reasoning was this. Say we do recovery every second and we dedup all these requests then we could do that for next ~50,000 years with a 32-bit counter. Why allocate more space when we are never going to hit the limit.
There was a problem hiding this comment.
I don't think it's worth drawing attention to, in order to save 4 bytes.
src/llfs/page_recycler_events.hpp
Outdated
| @@ -132,12 +142,14 @@ struct PackedPageToRecycle { | |||
|
|
|||
| little_page_id_int page_id; | |||
| little_u64 batch_slot; | |||
| u64 offset_as_unique_identifier; | |||
There was a problem hiding this comment.
Please use little_u64 here.
There was a problem hiding this comment.
sure... this part of the code is all changed now.
| @@ -111,13 +113,28 @@ Optional<SlotRange> PageRecyclerRecoveryVisitor::latest_info_refresh_slot() cons | |||
| return this->latest_info_refresh_slot_; | |||
| } | |||
|
|
|||
| //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - | |||
| // | |||
| slot_offset_type PageRecyclerRecoveryVisitor::volume_trim_offset() const | |||
There was a problem hiding this comment.
nit: Maybe have the member function name match the member data name? volume_trim_slot()?
There was a problem hiding this comment.
sure...thats a good point.
| @@ -45,6 +45,9 @@ class PageRecyclerRecoveryVisitor | |||
|
|
|||
| Optional<SlotRange> latest_info_refresh_slot() const; | |||
|
|
|||
| slot_offset_type volume_trim_offset() const; | |||
There was a problem hiding this comment.
Great! Consider grouping these together as a single struct. The advantages would be:
- Code that calls this API gets a very strong "hint" that the two always need to be called together (and the pair needs to travel around together)
- Can define custom operators for that type, to re-use the
slot_less_thanand lexicographical ordering that is coded inline in a couple places currently.
There was a problem hiding this comment.
Will do the change as discussed...
| @@ -29,6 +29,8 @@ namespace llfs { | |||
| , latest_batch_{} | |||
| , recycler_uuid_{boost::uuids::random_generator{}()} | |||
| , latest_info_refresh_slot_{} | |||
| , volume_trim_slot_{0} | |||
There was a problem hiding this comment.
Consider having these be Optional so that we can detect that case where no slot/index information could be recovered from the log. It's unlikely the slot will over overflow (being 64-bits), but making this Optional would also have the benefit of handling integer wrap (like slot_less_than).
There was a problem hiding this comment.
First time when the DB is coming up we will never find a slot/index during recovery. At that point we treat them as 0/0. I am not sure if making it optional will be useful here. Let me know if I missed anything?
This MR is to release the fix for ref count bug in page recycler's dead page recycling flow. This issue crops up when volume trimmer and page recycler retries a dead page recycling task for a page after recovery. The solution is to track largest_slot_offset in recycler which will be checked at the time of receiving any new dead_page recycling request from volume trimmer. Any request having lower or equal slot_offset compare to largest-slot-offset will be ignored.
The MR also adds in a histogram plot to show where all we are hitting the bug over the entire seed range. This is done through an existing page ref count test.
#13